JsonToRowTest: fix race condition - #36073
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a race condition by avoiding modification of global Jackson parameters. Instead of using overrideDefaultStreamReadConstraints, you've introduced a createJsonFactory method to create configured JsonFactory instances. This is a much safer, thread-safe approach. My review includes one high-severity comment on a bug in the new createJsonFactory method where a parameter is ignored, preventing it from working as described in the Javadoc.
|
Assigning reviewers: R: @ahmedabu98 for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| * factory to higher limits. If needed, call this method during pipeline run time, e.g. in | ||
| * DoFn.setup. This avoids a data race caused by modifying the global default settings. | ||
| */ | ||
| public static void increaseDefaultStreamReadConstraints(int newLimit) { |
There was a problem hiding this comment.
This is a breaking change. Do we know if users use this method or not?
Any chance we can keep both methods?
There was a problem hiding this comment.
I changed all of the instances inside beam, but for outside that not sure. Just noticed Templates does use it.
Will keep the old method, but take all known users off of it. Sound reasonable? Thanks.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request effectively addresses a race condition related to global Jackson parameter changes by introducing a JsonFactory with localized stream read constraints. This avoids modifying global state, making the JSON processing more robust and thread-safe. The changes in RowJsonUtils and TableRowJsonCoder are correct and consistent. I have one suggestion to improve the performance and robustness of the newly introduced createJsonFactory method by avoiding repeated reflection.
|
Run Java_Amazon-Web-Services2_IO_Direct PreCommit |
5e18249 to
844dc01
Compare
| private static class StreamReadConstraintsHelper { | ||
| static void setStreamReadConstraints(JsonFactory jsonFactory, int sizeLimit) { | ||
| com.fasterxml.jackson.core.StreamReadConstraints streamReadConstraints = | ||
| com.fasterxml.jackson.core.StreamReadConstraints.builder() | ||
| .maxStringLength(sizeLimit) | ||
| .build(); | ||
| jsonFactory.setStreamReadConstraints(streamReadConstraints); | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure why Gemini asked you to put this method in a nested class, but looks good either way.
|
Run Java_GCP_IO_Direct PreCommit |
|
Failure doesn't seem related. |
2 participants
Fix race condition dealing with global parameter changes.
b/436899963
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.